Skip to content

feat(rccl): integrate MADEngine workloads into CI [AICOMRCCL-1332] - #9055

Closed
prasanna-amd wants to merge 4 commits into
developfrom
users/prasanna-amd/rccl-ci-madengine
Closed

feat(rccl): integrate MADEngine workloads into CI [AICOMRCCL-1332]#9055
prasanna-amd wants to merge 4 commits into
developfrom
users/prasanna-amd/rccl-ci-madengine

Conversation

@prasanna-amd

@prasanna-amd prasanna-amd commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Add Llama-3.1-70B training workload (via MADEngine) to the RCCL CI pipeline on the Ruby cluster. A Docker overlay image swaps the CI-built librccl.so into a rocm/primus base container, enabling end-to-end validation of RCCL builds against real training workloads.

JIRA ID: AICOMRCCL-1332

Changes

  • test_madengine.py — orchestration: overlay build, manifest generation, SLURM dispatch, live log metric extraction, JSONL perf datastore with per-precision rolling regression detection
  • therock-rccl-test-madengine.yml — new workflow for MADEngine workloads
  • therock-rccl-ci.yml — schedule weekly → nightly, madengine_nodes input
  • therock-rccl-ci-linux.yml — wire in MADEngine job for gfx950
  • rccl_ci_utils.py, test_pytorch_c10d.py, test_jax_collective.py — CI scripts moved to projects/rccl/ci/scripts/

Review feedback addressed

  • Clones pinned: madengine at ec4de0b58c49, MAD at 688828bd9d4a on mad-rccl branch
  • HF token removed from manifest JSON (passed via MAD_SECRETS_HFTOKEN env only)
  • check_regression() now filters by precision (BF16/FP8 no longer share a rolling mean)
  • notify_webhookteams_webhook naming mismatch fixed in caller
  • PyTorch/JAX CI script fixes extracted to PR fix(rccl): replace pip-bundled librccl.so with CI-built version for PyTorch tests #9449

Validated results (single-node, 8x MI325X)

Precision TFLOP/s/GPU Tokens/s/GPU Iterations
BF16 773.9 1722.1 50/50
FP8 1249.4 2780.4 50/50

Test plan

  • Trigger workflow_dispatch to validate MADEngine workflow on Ruby
  • Validate single-node BF16/FP8 training completes 50/50 iterations
  • Confirm JSONL perf results written to /apps/rccl-ci/perf/madengine_results.jsonl
  • Multi-node (2N) validation (follow-up)

Copilot AI review requested due to automatic review settings July 22, 2026 22:15
@prasanna-amd
prasanna-amd requested review from a team as code owners July 22, 2026 22:15
@github-actions github-actions Bot added github actions Pull requests that update GitHub Actions code project: rccl labels Jul 22, 2026
@therock-pr-bot

therock-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass
Forbidden Files ✅ Pass
🧪 Unit Test ✅ Pass
🚫 Draft PR 🔜 To Be Enabled
🚩 Feature Flag 🔜 To Be Enabled
📊 Code Coverage 🔜 To Be Enabled

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates RCCL CI to run on a nightly cadence, refactors RCCL test scripts into projects/rccl/ci/scripts/, and adds a new MADEngine-based performance workload (with JSONL datastore + regression detection) for gfx950 on the Ruby cluster.

Changes:

  • Switch RCCL scheduled CI from weekly to nightly (06:17 UTC daily).
  • Refactor PyTorch/JAX CI workflows to use the new script location under projects/rccl/ci/scripts/.
  • Add MADEngine workload runner + reusable workflow to run Llama-3.1-70B training and record/compare throughput over time.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
projects/rccl/ci/scripts/rccl_ci_utils.py Adds shared helpers for artifact discovery, JUnit parsing, and notifications used by RCCL CI scripts.
projects/rccl/ci/scripts/test_pytorch_c10d.py Adds a PyTorch distributed c10d/NCCL test runner against CI-built RCCL.
projects/rccl/ci/scripts/test_jax_collective.py Adds a JAX collective smoke-test runner against CI-built RCCL with ROCm runtime setup.
projects/rccl/ci/scripts/test_madengine.py Adds MADEngine workload runner with perf.csv parsing, regression checks, and JSONL trend storage.
.github/workflows/therock-rccl-test-pytorch-distributed.yml Updates workflow to sparse-checkout and run PyTorch test script from the new scripts location.
.github/workflows/therock-rccl-test-jax-collective.yml Updates workflow to sparse-checkout and run JAX test script from the new scripts location.
.github/workflows/therock-rccl-test-madengine.yml Introduces a new workflow to run MADEngine workloads on Ruby and upload perf artifacts.
.github/workflows/therock-rccl-ci.yml Changes schedule from weekly to nightly.
.github/workflows/therock-rccl-ci-linux.yml Wires in the new MADEngine scheduled job for gfx950-dcgpu runs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread projects/rccl/ci/scripts/rccl_ci_utils.py
Comment thread projects/rccl/ci/scripts/test_pytorch_c10d.py
Comment thread projects/rccl/ci/scripts/test_jax_collective.py
Comment thread projects/rccl/ci/scripts/test_pytorch_c10d.py
Comment thread projects/rccl/ci/scripts/test_madengine.py
Comment thread projects/rccl/ci/scripts/test_madengine.py Outdated
Comment thread projects/rccl/ci/scripts/test_madengine.py Fixed
Comment thread projects/rccl/ci/scripts/test_madengine.py Fixed
@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

RCCL Perf-Regression Gate: ⚠️ NO VERDICT (not measured)

This run did not produce a usable answer. It is neither a PASS nor a regression — treat the perf gate as not run for this change.

Why:

  • this report predates the trustworthiness flag, or a group could not be scored

Mode: detect (reference vs candidate)
Thresholds: small 17.2% · mid 12.3% · large 12.9%
Keys compared: 0 · Confirmed regressions: 0 · Inconclusive: 0
Provenance: n/a

Per-collective breakdown
group keys regressions inconclusive
all_gather_perf-d=bfloat16-default 0 0 0
all_gather_perf-d=float-default 0 0 0
all_reduce_perf-d=bfloat16-default 0 0 0
all_reduce_perf-d=float-default 0 0 0
broadcast_perf-d=bfloat16-default 0 0 0
broadcast_perf-d=float-default 0 0 0
reduce_scatter_perf-d=bfloat16-default 0 0 0
reduce_scatter_perf-d=float-default 0 0 0

@mkuznet1 mkuznet1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see MADEngine wired into RCCL CI — this overlaps heavily with what we're
doing for Primus/SGLang multi-node in MAD, so a few notes to keep us on one path.

  1. Image distribution to compute nodes

build_rccl_overlay_image() re-implements docker save -> shared staging ->
per-node srun docker load. madengine already does exactly this natively when
the manifest has local_image: true — it saves the image to the MAD_DOCKER_BUILDS
shared dir, workers load the same tar, and it detects a stale image reused under
the same tag via the mad.build_fingerprint label:

https://github.com/ROCm/madengine/blob/ec4de0b58c49f05d89dd33e38cc3e81e0fb3d992/src/madengine/execution/container_runner.py#L2278-L2416

Could you try setting MAD_DOCKER_BUILDS to your shared staging dir and dropping
the custom distribution? If it works on Ruby we'd be on a single mechanism, and
you also get the staleness check for free.

  1. Manifest shape + bootstrap interface

The generated manifest puts slurm / env / model_repo / container_mounts at the
top level, but madengine reads deployment_config.{target,slurm,distributed,env_vars}
(see cli/validators.py:154 and primus_backend.py:36). Those top-level blocks are
inert today — the values that actually take effect are the ones duplicated in
--additional-context. Worth moving them into deployment_config so the manifest is
the single source of truth.

Related: nothing sets NCCL_SOCKET_IFNAME / slurm.network_interface. job.sh.j2
only exports it when slurm.network_interface is present, so NCCL bootstrap iface
is left to auto-detection. On Ruby the data-plane ifaces are benic*p1 /31
point-to-point links, so the bootstrap iface should be pinned to fenic0.

We have a static manifest validator that catches both of these (dead config,
missing iface, nodes vs nnodes, leftover placeholders) plus per-cluster env
templates, in https://github.com/ROCm/MAD-private/pull/390 :
.claude/skills/mad-slurm-multinode/scripts/validate_manifest.sh
.claude/skills/mad-slurm-multinode/assets/mad.env/mad.env.thor2-bnxt.template
.claude/skills/mad-slurm-multinode/references/cluster-types.md
Feel free to reuse them.

  1. Two things that will break the nightly
  • args passes the card name as --model_repo
    ("primus_pyt_megatron_lm_train_llama-3.1-70b_scaleout"), but
    scripts/primus/megatron-lm/run.sh matches base names only
    ("primus_pyt_megatron_lm_train_llama-3.1-70b") with no _scaleout branch and no
    fallback, so $model ends up empty. The model card already carries the correct
    args — dropping the override should fix it.

  • Heads-up on a rename: https://github.com/ROCm/MAD-private/pull/388 renames
    those cards from *_scaleout to *_overlay, which will break the hardcoded
    model_repo in WORKLOAD_CONFIGS once it lands. Let's coordinate the timing.

@prasanna-amd
prasanna-amd force-pushed the users/prasanna-amd/rccl-ci-madengine branch from ecdb82e to f0b8e74 Compare July 31, 2026 17:33
@prasanna-amd

Copy link
Copy Markdown
Contributor Author

@mkuznet1 Thanks for the thorough review — great catches across the board. Addressed all three in commit 4c3b573:

1) Image distribution — Great call on MAD_DOCKER_BUILDS. We now set it to work_dir/docker_builds so madengine's native tar-based distribution handles multi-node overlay image propagation. Registry push kept as optional fallback. The manifest already had local_image: true so this was a straightforward wire-up.

2) Manifest shape / NIC config — Added slurm.network_interface (set from NCCL_SOCKET_IFNAME) to both the manifest and --additional-context, so the SLURM template properly exports NCCL_SOCKET_IFNAME and GLOO_SOCKET_IFNAME via its {% if network_interface %} block. Also completed the Ruby NIC config: added NCCL_IB_TC=104 and NCCL_IB_QPS_PER_CONNECTION=4, and fixed NCCL_SOCKET_IFNAME to include both fenic0,enp49s0f0np0 (both needed for bootstrap on Ruby). Will look into the manifest validator and cluster env templates you linked — those would be a nice improvement for a follow-up.

3) Model naming — Our WORKLOAD_CONFIGS currently uses the plain base names (e.g. primus_pyt_megatron_lm_train_llama-3.1-70b) which match run.sh today. To handle the upcoming _scaleout_overlay rename in MAD-private #388, we added model_repo_aliasesgenerate_manifest() now checks run.sh for the primary name, and if not found, falls back to _overlay then _scaleout variants. This way the nightly won't break regardless of which side of the rename lands first.

@prasanna-amd
prasanna-amd force-pushed the users/prasanna-amd/rccl-ci-madengine branch from 4c3b573 to 94110d6 Compare July 31, 2026 22:57

@mkuznet1 mkuznet1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this through — 30 fix commits against a cluster madengine had never seen is a
genuinely useful stress test, and nearly everything you had to work around is a real madengine gap
rather than anything wrong on your side.

One request up front, and it is the main point of this review: please don't merge this as-is.
madengine is about to get a lot of active development, so anything that lives here as a patch
against madengine's source will rot — quietly, on a nightly job that no PR check exercises. I'd
like as much of this as possible to move into madengine itself, and only the genuinely CI-specific
parts to stay here.

Before the fix list: run against mad-rccl, not develop

mad-rccl is the MAD branch this CI should be tracking, once
ROCm/MAD#194 lands on it. That branch exists to carry the
RCCL-facing benchmarks; it currently sits at develop's tip (f7e2560, i.e. after Primus v26.5) and
#194 is open against it. It is prepared in parallel with your work and validated on three cluster
archetypes, and a large part of what this PR builds by hand is already in it. Concretely:

  • The overlay Dockerfiles — please use these rather than rebuilding the mechanism.
    docker/primus_megatron_train_rccl_overlay.ubuntu.amd.Dockerfile builds RCCL from source over
    rocm/primus:v26.5, takes repo/branch/commit as build args and verifies at build time that
    every librccl on disk matches the built SHA, with optional rdma-core for Broadcom Thor2.
    docker/sglang_disagg_inference_full_overlay.ubuntu.amd.Dockerfile does the same for
    RCCL + MoRI + NIXL/Mooncake in one build. That is the same job as the readelf kpack sniffing,
    the hardcoded ROCm-SDK venv paths and the in-place librccl overwrite at :385-439 — except it
    is a build-time overlay with a verified SHA rather than a post-hoc file swap, and madengine can
    drive it directly through context.docker_build_arg. Swapping a library into a finished image
    and hoping the loader picks it up is the part I would most like to see gone; you already found
    one edge of it when LD_PRELOAD had to be reverted for the LLVM symbol clash.
  • The Megatron-LM harness and its metric parsers, scripts/primus_megatron-lm/, writing
    madengine's perf-CSV schema directly. That is what the live-log regex at :681-739 substitutes
    for.
  • Real model cardsprimus_pyt_megatron_lm_train_llama-3.1-{8b,70b,405b}_overlay in
    models.json. Your workload is primus_pyt_megatron_lm_train_llama-3.1-70b, which is the same
    thing; with a card you would not need to fabricate built_images, built_models and context
    at all.
  • Cluster templates and a bring-up skill for CX7/Mellanox-RoCE, AMD-AINIC/Pollara and
    Broadcom-Thor2-RoCE: mad.env plus manifest templates with the transport variables already
    worked out per archetype.

And the one that bears directly on the exit-code discussion below: #194's last commit fixes a
Primus v26.5 field rename that made both metric parsers match nothing. The symptom is exact —
training completes, SLURM reports COMPLETED, every per-node CSV holds only a header, and perf.csv
comes out empty. If you are on a v26.5 base, that is very plausibly the real reason madengine
reports failure on a successful run.

So the concrete ask: point the MAD clone at mad-rccl (pinned to a SHA or tag, per the question
below) instead of public develop, and build the competitor image from the overlay Dockerfile
there instead of assembling one in the CI script. Could we sync before you invest more in the
local versions? It would delete a large part of this diff rather than relocate it.

Moving the fixes into madengine

Two ways to do it, whichever is less work for you:

  • hand them to me and I'll land them in madengine, or
  • open the PRs against madengine yourself and add @mkuznet1, @i-kosarev and @coketaste as reviewers.

I realise this makes the current task bigger rather than smaller. I still think it is the right
call: this CI integration is a long-lived project, and the alternative is maintaining a patch set
against a moving target.

What I'd like to move upstream. Already written on our side, so close to free — we hit the
same wall on the same cluster, independently of this PR:

  • --gpus-per-node emitted unconditionally (job.sh.j2:9) — becomes a manifest flag honoured in
    both the template and the script builder, rather than a line deletion
  • the 5 s madengine --version validation timeout (slurm.py:244), too tight for an NFS-backed
    interpreter — raised
  • yum info rocm-libs hanging on a GPG prompt (run_orchestrator.py:770) — capped with a
    timeout for every package manager rather than swapped for rpm -qi, since the underlying
    problem is a tty-less prompt rather than yum specifically

Small, not yet written:

  • $HOME/.local/bin missing from the sbatch PATH (job.sh.j2:39)
  • the shared-FS probe not matching nfs4 (job.sh.j2:144)
  • MAD_DOCKER_BUILDS being discoverable only by reading the source

Needs a little design, but still ours:

  • running a pre-built image without having to fabricate built_images, built_models and a
    context block for a run that never builds anything
  • a headless submit-node mode, so you don't have to invent gpu_renderDs and a HIP version to get
    past the GPU-context guards. Those invented values are the right call given what madengine
    offers today, but they'll be wrong the day the cluster changes and nothing will say so
  • a Primus/Megatron perf parser emitting one row per precision, which is what would let the
    live-log scraping go away
  • separating "the workload failed" from "metric collection failed", which is what forces the
    exit-code override

What should stay here: the JSONL perf store and the rolling regression gate. madengine has no
perf-history or regression concept in its design and I don't intend to add one — that policy
belongs to the consumer, which is where you already have it.

On the patching helper specifically

patch_madengine_for_cluster() (lines 166-314) rewrites madengine's own
templates/slurm/job.sh.j2, deployment/slurm.py and orchestration/run_orchestrator.py by
string match, after a git clone --depth=1 of madengine's default branch with no pin. Two risks
compound: the clone is unpinned, and every patch fails open — on a miss it logs "already patched
or not found" and continues, so an upstream rename becomes a silent no-op that surfaces days later
as a confusing SLURM error. Given the amount of change coming to madengine, that is close to a
guarantee rather than a risk.

Could you pin the madengine clone to a SHA in the meantime? That alone turns the fail-open
patches from a latent surprise into a deterministic one. The pre-warm at :633-642 also becomes
unnecessary once the probe timeout is raised.

The same goes for the MAD clone (:132-137), and I think it removes the need for the
run.sh grep at :488-499. --model_repo is a MAD-side argument — run.sh parses it at line 33
and the accepted spellings are a MAD contract; madengine never reads it, it just forwards whatever
args says. So checking whether your name is a substring of the script, and falling back through
an alias list if not, is really a hedge against MAD moving under you. Pin MAD to a tag or SHA and
the name is simply known. I'd rather not solve this by having madengine resolve names against
models.json — the manifest carrying the name explicitly is the design we want, not something to
paper over.

On the per-node venv (lines 224-287) — is the Python split actually unavoidable? A venv is
only a thin overlay on the interpreter that created it: it points at the head node's
python3.10 and its compiled extensions are tagged cpython-310, so a 3.9 compute node cannot
use it. That is exactly what you hit, and the block works around it by creating a second venv per
node from that node's own python3 and reinstalling madengine into it.

A conda env doesn't have that property — it ships its own interpreter, so if the env lives on the
shared FS every node sees the same Python and the same site-packages, and there is nothing to
bootstrap per node. That is what we use for multi-node runs and it holds across three different
cluster archetypes:

conda env list | grep -q '^madenv ' || conda create -y -n madenv python=3.12
conda activate madenv
pip install -e ./madengine

with the env on the shared work FS and conda sourced in the job script. If that fits your runner,
the whole per-node install disappears — and with it a pip install on the critical path of every
node — without needing anything from madengine at all. Worth a try before we treat it as an
upstream gap.

Questions before merge — I may be wrong on these

Both of the following come from reading the workflows rather than from running them, and you have
clearly been running this against a real cluster, so treat them as questions.

1. Does the scheduled job actually dispatch? therock-rccl-ci-linux.yml:229 passes
notify_webhook, while therock-rccl-test-madengine.yml declares teams_webhook (lines 23 and
50) and reads inputs.teams_webhook at 156-157. The sibling pytorch/jax jobs at lines 188, 201
and 214 pass notify_webhook because their callees declare it. Reading it cold, that looks like
an undeclared workflow_call input, which would fail validation before the job starts — and since
the job is gated on schedule/workflow_dispatch plus gfx950-dcgpu, no PR check would tell us.
If you've already seen it dispatch, ignore this.

2. Two questions on the HF token. First, is it reaching the job at all?
therock-rccl-test-madengine.yml:147 reads ${{ secrets.HF_TOKEN || '' }}, but I can't find a
secrets: block or secrets: inherit on the call in therock-rccl-ci-linux.yml, which would
make it empty on the scheduled path and fail gated Llama-3.1 access at runtime rather than at
startup.

Second, and more important: can you confirm the token doesn't land anywhere durable? It currently
goes into env_vars (test_madengine.py:486) → into the manifest (:563) → written to disk at
:570, and therock-rccl-test-madengine.yml:178 uploads manifest.json with
retention-days: 30, which is what CodeQL 768 is pointing at (769, the logging one, was fixed by
5631aaae). We had a leak of this kind in madengine and added redaction for MAD_SECRETS_* in
printed commands (fcc4905), but that only covers what madengine itself prints — it won't help
with a manifest written by the CI script and uploaded as an artifact. Passing the token through
the process environment only, and keeping it out of the manifest, would close that off; madengine
picks up MAD_SECRETS_* from the environment, so the manifest doesn't need to carry it.

3. Which MAD are you actually cloning, and is the script copy doing anything? :135 clones
https://github.com/ROCm/MAD.git at --depth=1 with no branch, so that is public MAD's default
branch — develop, unpinned, and it moved as recently as yesterday. Two things there I can't
reconcile:

install_madengine() copies MAD/scripts/primus into the work dir (:147-152), but that path no
longer exists on develop: #187 (Primus v26.5, merged
2026-08-01) deleted scripts/primus/megatron-lm/ in favour of the scripts/Primus submodule —
which is empty after a non-recursive --depth=1 clone — and a convention-based
scripts/primus_train/. The copy is guarded by if scripts_src.is_dir() with no else, so on a
clean checkout it is skipped in silence, and then run_sh.exists() at :491 is false, which makes
the alias fallback underneath it unreachable.

Meanwhile the manifest declares "scripts": "scripts/primus/megatron-lm/run.sh" (:518), and
--work-dir points at a persistent path on the cluster. My guess is that tree was populated once,
by hand or by an older MAD layout, and has been there ever since — which works right up until the
runner is rebuilt.

The fix is the one above: clone mad-rccl at a pinned SHA, where the Megatron-LM harness lives as
scripts/primus_megatron-lm/ and is maintained against v26.5. And whichever ref you end up on,
the copy should fail loudly when its source directory is missing rather than skip — a silent
no-op here is what let this survive a breaking upstream restructure without a single error line.

Worth re-checking

4. Does check_regression() separate precisions? 5b1af729 started writing one record per
precision run, but the filter at test_madengine.py:764-784 looks like it keys on workload, scale
and status only. If so, BF16 and FP8 share one rolling 5-run mean and the −2% gate fires on a
change in precision mix rather than on a regression. Could you double-check whether precision
needs to be in the filter key?

5. The exit-code override, and the empty perf underneath it.
test_madengine.py:1066-1083 forces exit 0 whenever every training run reached its final iteration.
I understand exactly why it is there, but I think the empty perf it compensates for has a specific,
fixable cause: the model entry at :513-524 doesn't declare multiple_results.

That field is how madengine learns where the numbers are. When it is set, madengine passes the
filename into the container as MAD_OUTPUT_CSV, the model script writes that CSV, and madengine
turns each row into a perf.csv row — which is also how you get one row per precision without
scraping anything. When it is absent, madengine falls back to grepping the log for a literal
performance: <value> <metric> line, and Primus never prints one. So performance comes out empty
and the run is marked failed even though training succeeded. The producer for that CSV is the
Megatron-LM report script, which is the other half of question 3 — it isn't on develop any more,
and it is in mad-rccl.

Setting multiple_results and running against mad-rccl should remove the need for both the
override and the live-log regex. Separately, madengine genuinely cannot today distinguish "the
workload failed" from "metrics were empty", and that is on our side to fix; as written the override
will also swallow a real node failure that coexists with a completed training loop. Could you narrow
it to the specific empty-perf condition and log loudly whenever it triggers, until we do?

One note

Thanks for switching to MAD_DOCKER_BUILDS and for completing the NIC config with
slurm.network_interface — both are the right way to use madengine, and the fact that neither is
discoverable without reading the source is on us to fix.

@prasanna-amd prasanna-amd changed the title feat(rccl): integrate MADEngine workloads, nightly cadence, and CI refactor [AICOMRCCL-1332] feat(rccl): integrate MADEngine workloads into CI [AICOMRCCL-1332] Aug 5, 2026
Add Llama-3.1-70B training workload (via MADEngine) to the RCCL CI
pipeline on the Ruby cluster. The mechanism is a Docker overlay image
that swaps the CI-built librccl.so into a rocm/primus base container,
enabling end-to-end validation of RCCL builds against real training
workloads without rebuilding the full PyTorch/ROCm stack.

Key components:
- test_madengine.py: orchestration script — overlay build, manifest
  generation, SLURM dispatch, live log metric extraction, JSONL perf
  datastore with per-precision rolling regression detection
- therock-rccl-test-madengine.yml: workflow for MADEngine workloads
- CI schedule changed from weekly to nightly (06:17 UTC)
- CI scripts moved to projects/rccl/ci/scripts/ (from .github/scripts/)

Clones pinned: madengine at ec4de0b58c49, MAD at 688828bd9d4a on
the mad-rccl branch. HF token passed via process environment only
(not written to manifest artifact).

Validated single-node (8x MI325X):
  BF16: 773.9 TFLOP/s/GPU, 1722 tokens/s/GPU (50/50 iterations)
  FP8:  1249.4 TFLOP/s/GPU, 2780 tokens/s/GPU (50/50 iterations)
@prasanna-amd
prasanna-amd force-pushed the users/prasanna-amd/rccl-ci-madengine branch from 7489c59 to 3b5bbd0 Compare August 5, 2026 22:15
Fixes discovered during manual validation on Ruby cluster:

- Add --network=none to Docker overlay build (bridge not available
  on all compute nodes)
- Pin SLURM job to the overlay build node when no registry is
  configured (image only exists locally)
- Disable madengine node health check that overrides nodelist
- Strip multi-NIC config for single-node runs (Gloo requires all
  listed NICs to exist, unlike NCCL)
- Fix MAD scripts path (primus_megatron-lm, not primus/megatron-lm)
- Use full SHA for MAD pin
- Smart MAD clone: verify HEAD before fetch+checkout
- Handle PermissionError when saving run artifacts to shared dir

Validated: 1N/8GPU llama-3.1-70b-training on Ruby
  BF16: 763.2 TFLOP/s/GPU, 1698.4 tok/s/GPU
  FP8:  1254.7 TFLOP/s/GPU, 2792.2 tok/s/GPU
@mkuznet1

mkuznet1 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Found the root cause of the empty perf / FAILURE-on-success here, and it's a madengine legacy
trap rather than anything you did wrong.

The built_models entry in the generated manifest (test_madengine.py:532-551) doesn't declare
multiple_results. That field tells madengine the workload reports through a CSV; without it,
madengine silently falls back to grepping the log for a performance: <value> <metric> line,
which Primus never prints. Result: empty metric, FAILURE, no error anywhere. That single field is
upstream of the live-log scraping and the exit-code override.

Quickest unblock: copy the built_models entry from the MAD model card in mad-rccl — it already
carries the right multiple_results and scripts path — then freeze that manifest as the CI
template.

We're fixing the silent-degradation half in madengine (AICOMNET-366); will follow up here.

@mkuznet1 mkuznet1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking up the previous round — pinning both clones and moving MAD onto the mad-rccl branch is exactly what we were after. It also means the overlay card and its multiple_results are now available to you directly, instead of having to be reconstructed.

Please model generate_manifest on the manifests we use for manual multinode runs — same MAD branch you pin: https://github.com/ROCm/MAD/tree/mad-rccl/.claude/skills/mad-slurm-multinode/assets/manifests (primus_llama-3.1-70b) — slurm/distributed/env_vars under deployment_config, mounts in context.docker_mounts, multiple_results on the card. Root-level keys are ignored.

Comment on lines +557 to +575
"built_models": {
image_key: {
"name": model_repo,
"tags": ["pyt", "pretrain", "training"],
"dockerfile": "N/A (overlay image)",
"scripts": f"scripts/{scripts_dir.name}/run.sh",
"n_gpus": str(total_gpus),
"owner": "",
"training_precision": workload_config.get("precision", ""),
"args": f"--model_repo {model_repo}",
"additional_docker_run_options": (
"--device=/dev/infiniband"
if cluster_config.get("mount_host_ib_libs") and nodes > 1
else ""
),
"data": "",
"cred": "",
"timeout": None,
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

built_models re-declares the MAD card by hand and drops multiple_results (the pinned card has perf_primus-megatron-Llama-3.1-70B.csv). Without it madengine falls back to the performance: log regex, finds nothing, and writes an empty row with FAILURE — root cause of the live-log scraper (739) and the exit-code override (1145). Load the card from models.json; override only the image.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Added multiple_results field to built_models card, pointing to perf_primus-megatron-Llama-3.1-70B.csv. Validated in Run 13 — perf_entry_super.json is now populated with per-precision rows.

Comment on lines +597 to +608
"slurm": {
"partition": cluster_config.get("slurm_partition", workload_config["slurm_partition"]),
"qos": cluster_config.get("slurm_qos", ""),
"network_interface": nccl_env.get("NCCL_SOCKET_IFNAME", ""),
"nodes": nodes,
"ntasks_per_node": gpus_per_node,
"gpus_per_node": gpus_per_node,
"time": workload_config["time_limit"],
"exclusive": True,
"enable_node_check": False,
**({"nodelist": nodelist} if nodelist else {}),
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

madengine reads deployment config from manifest["deployment_config"] (run_orchestrator.py:225,232) and from --additional-context; a top-level slurm key is ignored. So qos and ntasks_per_node never apply — and the block you pass at 653-662 has no qos, so the job runs without --qos=vip_prio. Nest this under deployment_config, or drop it and keep one source.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Restructured manifest: slurm config now under deployment_config.slurm, removed the redundant top-level slurm key and the duplicate additional_context block in run_madengine().

Comment on lines +517 to +524
env_vars = {
**nccl_env,
"NCCL_DEBUG": "WARN",
}

if cluster_config.get("mount_host_ib_libs") and nodes > 1:
if "NCCL_SOCKET_IFNAME" in nccl_env:
env_vars["GLOO_SOCKET_IFNAME"] = nccl_env["NCCL_SOCKET_IFNAME"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manifest["env"] is not read by madengine (no reference in the source), so all of env_vars is dead — including GLOO_SOCKET_IFNAME, which is set only here (522-524) and never added to context.docker_env_vars (582-591). For nodes>1 Gloo is left to autodetect on a host with 8 bnxt_re NICs plus fenic0. Move it into docker_env_vars.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Moved GLOO_SOCKET_IFNAME into context.docker_env_vars. Top-level env block removed.

Comment on lines +612 to +613
if workload_config.get("container_mounts"):
manifest["container_mounts"] = workload_config["container_mounts"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

container_mounts is not a madengine manifest key either — nothing in the source references it, so /shared:/shared is never mounted into the container. Use context.docker_mounts (currently {} at L592) or additional_docker_run_options.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Mounts now in context.docker_mounts. Top-level container_mounts removed.

"tags": ["pyt", "pretrain", "training"],
"dockerfile": "N/A (overlay image)",
"scripts": f"scripts/{scripts_dir.name}/run.sh",
"n_gpus": str(total_gpus),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n_gpus = str(total_gpus) is 16 on a 2-node run. n_gpus is a per-node field; the MAD card sets -1 deliberately so madengine resolves it per node. Passing the cluster-wide total here is at best redundant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Set n_gpus: "-1" so madengine resolves per-node.

run_artifacts = results_dir / "runs" / run_id
try:
run_artifacts.mkdir(parents=True, exist_ok=True)
subprocess.run(["cp", str(output_csv), str(run_artifacts / "perf.csv")], check=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subprocess.run(..., check=True) raises CalledProcessError, which is not an OSError — so the handler below catches the mkdir failure but not the cp failure it is aimed at. Use shutil.copy2, or catch subprocess.CalledProcessError too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Switched to shutil.copy2() instead of subprocess.run(["cp", ...]). Exception handler now catches OSError which covers both mkdir and copy failures.

Comment on lines +1184 to +1187
is_regression, regression_msg = check_regression(
results_dir, args.workload, scale, metric_value,
workload_config["type"],
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback path calls check_regression without precision, so it compares only against records whose stored precision is null — a baseline disjoint from the live-log path. Worth normalising the key or stating the intent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. The fallback path (no live_log_runs) is intentionally a coarser check — it runs when structured results exist but live logs don't. In that case precision is unknown, so it compares against the null-precision baseline. Will normalize once perf_entry_super is the sole source.

Comment on lines +1107 to +1117
# Step 6: Parse results — try perf.csv first, fall back to live log
metric_value = None
metric_key = workload_config["metric_key"]
if output_csv.exists():
perf_data = parse_perf_csv(output_csv, metric_key)
if metric_key in perf_data and perf_data[metric_key]:
try:
metric_value = float(perf_data[metric_key])
log.info("Metric %s = %.1f (from perf.csv)", metric_key, metric_value)
except (ValueError, TypeError):
log.warning("Could not parse metric %s: %s", metric_key, perf_data.get(metric_key))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read perf_super.csv/perf_super.json rather than the flat perf.csv. The flat file has no stable schema — its columns come from whatever the model emitted, so width varies per card (38 columns on our 2N Primus run). perf_super is constant-width (31 columns) with model-specific dimensions folded into multi_results as JSON, one object per row. It is only populated when multiple_results is set (see 557-575).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From perf_super you get straight from madengine what this file rebuilds by hand: multi_results.precision for per-precision keying (instead of scraping the Running: header at 739), per-row status, scale via nnodes/gpus_per_node/n_gpus, madengine's own relative_change, and provenance — docker_image, git_commit, build_number (the SLURM job id) — for the datastore.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. parse_perf_results() now reads perf_entry_super.json first (31 fixed columns), falls back to perf.csv only if the super file is missing. Also copying perf_entry_super.* files to run artifacts dir.

Comment on lines +1119 to +1125
run_id = os.environ.get("GITHUB_RUN_ID", "local")
run_artifacts = results_dir / "runs" / run_id
try:
run_artifacts.mkdir(parents=True, exist_ok=True)
subprocess.run(["cp", str(output_csv), str(run_artifacts / "perf.csv")], check=True)
except OSError as exc:
log.warning("Could not save run artifacts to %s: %s", run_artifacts, exc)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a single run read perf_entry_super.csv/.json — same 31 columns as perf_super.* but only this run's rows, so no build_number filtering is needed; perf_super.* is the cumulative file. Both pairs are written under fixed names into the run cwd (work_dir, per cwd=work_dir at 701) — -o names only the flat CSV. Copy them into results_dir/runs/<run_id>/ next to perf.csv.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Now reading perf_entry_super.json (single-run rows). Copying both perf_entry_super.csv and perf_entry_super.json to results_dir/runs/<run_id>/. Also added to the workflow artifact upload step.

Comment on lines +201 to +349
def patch_madengine_for_cluster(
madengine_dir: Path,
no_gres: bool = False,
) -> None:
"""Patch madengine source for cluster-specific compatibility."""
src = madengine_dir / "src" / "madengine"

if no_gres:
template = src / "deployment" / "templates" / "slurm" / "job.sh.j2"
if not template.exists():
log.warning("SLURM template not found at %s", template)
else:
content = template.read_text()
patched = content.replace(
"#SBATCH --gpus-per-node={{ gpus_per_node }}\n", ""
)
if patched != content:
template.write_text(patched)
log.info("Patched SLURM template: removed --gpus-per-node directive")
else:
log.info("SLURM template already patched (no --gpus-per-node)")

template = src / "deployment" / "templates" / "slurm" / "job.sh.j2"
if template.exists():
content = template.read_text()
marker = "# Load required modules"
if marker in content and "$HOME/.local/bin" not in content:
patched = content.replace(
marker,
'export PATH="$HOME/.local/bin:$PATH"\n\n' + marker,
)
template.write_text(patched)
log.info(
"Patched SLURM template: added $HOME/.local/bin to PATH "
"(SLURM jobs do not inherit user shell PATH)"
)
else:
log.info("SLURM template PATH patch already present or marker not found")

slurm_py = src / "deployment" / "slurm.py"
if slurm_py.exists():
content = slurm_py.read_text()
patched = content.replace(
'["madengine", "--version"],\n'
" capture_output=True,\n"
" text=True,\n"
" timeout=5,",
'["madengine", "--version"],\n'
" capture_output=True,\n"
" text=True,\n"
" timeout=120,",
)
if patched != content:
slurm_py.write_text(patched)
log.info("Patched slurm.py: increased CLI validation timeout to 120s")
else:
log.info("slurm.py already patched or timeout string not found")

template = src / "deployment" / "templates" / "slurm" / "job.sh.j2"
if template and template.exists():
content = template.read_text()
# Patch the MULTI-NODE verification block (inside TASK_SCRIPT_EOF
# heredoc) to install madengine per-node when the head node's venv
# is incompatible (Python 3.10 vs 3.9). The single-node block
# runs on the head node where the venv works — leave it alone.
#
# Find the multi-node block by searching for the verification
# string AFTER the TASK_SCRIPT_EOF heredoc marker.
heredoc_marker = "TASK_SCRIPT_EOF"
heredoc_idx = content.find(heredoc_marker)
if heredoc_idx != -1:
verify_str = 'echo "Verifying madengine availability..."'
mn_verify_idx = content.find(verify_str, heredoc_idx)
if mn_verify_idx == -1:
mn_verify_idx = content.find(verify_str)
if mn_verify_idx != -1:
mn_end_str = "# Create local execution manifest"
mn_end_idx = content.find(mn_end_str, mn_verify_idx)
if mn_end_idx != -1:
replacement = (
'echo "Verifying madengine availability..."\n'
'MAD_CLI_COMMAND=""\n'
'if command -v madengine >/dev/null 2>&1 && '
'madengine --help >/dev/null 2>&1; then\n'
' MAD_CLI_COMMAND="madengine"\n'
' echo " ✓ madengine available: '
'$(madengine --version 2>&1 | head -1)"\n'
'fi\n'
'if [ -z "$MAD_CLI_COMMAND" ]; then\n'
' echo " ⚠ madengine not functional — '
'installing for this node\'s Python ($(python3 --version))"\n'
' SUBMISSION_DIR={{ manifest_file | dirname }}\n'
' MADENGINE_SRC="$SUBMISSION_DIR/madengine"\n'
' if [ -d "$MADENGINE_SRC" ] && [ -f "$MADENGINE_SRC/pyproject.toml" ]; then\n'
' python3 -m venv "$WORKSPACE/node_venv"\n'
' source "$WORKSPACE/node_venv/bin/activate"\n'
' pip install --upgrade pip setuptools wheel 2>&1 | tail -3\n'
' pip install "$MADENGINE_SRC" 2>&1 | tail -20\n'
' if madengine --version >/dev/null 2>&1; then\n'
' MAD_CLI_COMMAND="madengine"\n'
' echo " ✓ madengine installed: '
'$(madengine --version 2>&1 | head -1)"\n'
' else\n'
' echo " ✗ madengine install failed"\n'
' exit 1\n'
' fi\n'
' else\n'
' echo " ✗ madengine source not found at $MADENGINE_SRC"\n'
' exit 1\n'
' fi\n'
'fi\n'
'echo ""\n\n'
)
content = content[:mn_verify_idx] + replacement + content[mn_end_idx:]
template.write_text(content)
log.info("Patched SLURM template: added per-node madengine install (multi-node)")
else:
log.warning("Could not find end of multi-node verification block")
else:
log.warning("Could not find multi-node verification block in template")
else:
log.warning("TASK_SCRIPT_EOF not found — template may not have multi-node support")

template = src / "deployment" / "templates" / "slurm" / "job.sh.j2"
if template and template.exists():
content = template.read_text()
old_nfs_pattern = r"\bnfs\b"
new_nfs_pattern = r"\bnfs[0-9]*\b"
if old_nfs_pattern in content and new_nfs_pattern not in content:
content = content.replace(old_nfs_pattern, new_nfs_pattern)
template.write_text(content)
log.info("Patched SLURM template: NFS detection now matches nfs4")

run_orch = src / "orchestration" / "run_orchestrator.py"
if run_orch.exists():
content = run_orch.read_text()
patched = content.replace(
'print(self.console.sh("yum info rocm-libs", canFail=True))',
'print(self.console.sh("rpm -qi rocm-libs 2>/dev/null '
'|| echo rocm-libs not installed as RPM", canFail=True))',
)
if patched != content:
run_orch.write_text(patched)
log.info(
"Patched run_orchestrator.py: replaced 'yum info' with 'rpm -qi' "
"to avoid interactive GPG prompt hang"
)
else:
log.info("run_orchestrator.py already patched or yum string not found")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this function gone — patching installed madengine by string replacement fails soft (each branch warns and continues), so a madengine bump silently un-patches you. We're upstreaming most of it now (PATH, nfs4, --gpus-per-node, the yum hang) under AICOMNET-366; the per-node install is a genuine madengine gap we'll take as a feature request. Keep it isolated so it can be dropped in one commit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — keeping it isolated in a single function so it can be dropped in one commit once the upstream patches land (AICOMNET-366). The per-node install gap is real and appreciated as a feature request.

return False


def build_rccl_overlay_image(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. :400 + :403 — the tag identifies the wrong commit, so the image freezes. get_rccl_commit() reads RCCL_COMMIT_HASH, which nothing in this PR ever sets, then falls back to git rev-parse HEAD with no cwd — i.e. $GITHUB_WORKSPACE, which the first checkout populates with ROCm/TheRock pinned at 2e7c190. The tag is therefore a constant, and the docker image inspect short-circuit at :403 turns that into a cache hit on a persistent runner: after the first nightly, every run reuses the first night's librccl and reports green. The same wrong SHA is recorded as provenance in the JSONL store.

  2. :485 — the push tag is not a valid Docker reference, on the scheduled path. rocm/primus:v26.4 → rocm-primus:v26.4, giving ghcr.io/…/rccl-ci:rocm-primus:v26.4-; a tag cannot contain :, so docker tag fails under check=True. The workflow passes --registry ghcr.io/rocm/rocm-systems, so this is not latent. It also drops gpu_target, which the local tag carries — gfx942 and gfx950 would overwrite each other.

  3. :469 — the non-kpack fallback is unreachable and the swap can be a silent no-op. readlink -f exits 0 for a non-existent final component, so when /opt/rocm/lib/librccl.so is absent the || find … never runs and cp creates a stray file instead of overwriting the versioned library the loader maps. On a primus:v26.4 base ROCm lives under _rocm_sdk_libraries/lib and _rocm_sdk_devel/lib, so /opt/rocm/lib/librccl.so.1 may not exist at all and the run quietly measures stock RCCL. No pipefail, no existence check.

  4. Nothing verifies the swap. verify_rccl_override() is used only by the pytorch/jax scripts and only asserts a file exists on disk. Combined with 1 and 3, every failure mode above is silent.

  5. :446-448, :417. The SDK paths hardcode python3.12 and skip torch/lib. The staging copy skips when the destination exists, so a reused --work-dir ships the previous run's library under a new tag — safe in CI, where WORK_DIR carries github.run_id, but not for the manual runs the validation table came from.

Rather than hardening this, I would recommend to take docker/primus_megatron_train_rccl_overlay.ubuntu.amd.Dockerfile from the mad-rccl branch you already pin: it takes repo/branch/commit and BUILD_GPU_TARGETS as build args, madengine drives it through context.docker_build_arg, and it asserts at build time that every librccl on disk carries the built SHA. That replaces the readelf kpack sniffing, the hardcoded venv paths and the in-place overwrite with a build-time overlay whose provenance is checked rather than assumed — swapping a library into a finished image and hoping the loader picks it up is the part worth deleting. One gap to close in the move: that Dockerfile builds RCCL from source, so if CI must inject the prebuilt TheRock artifact instead, add that as an input to the same Dockerfile so the SHA gate still applies to it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough review. Addressed items 1, 2, 4 in a3089bc; items 3 and 5 acknowledged:

  1. Provenance/cache hit — Fixed. get_rccl_commit() no longer falls back to git rev-parse HEAD. Resolution order is now: RCCL_COMMIT_HASH env → GITHUB_RUN_ID → sha256 of librccl.so. Each CI run produces a unique tag.

  2. Push tag double-colon — Fixed. base_image.replace("/", "-").replace(":", "-") produces a valid Docker reference.

  3. Non-kpack fallback — Acknowledged. On primus:v26.4 the kpack path is always taken (TheRock builds with kpack), so the non-kpack branch is effectively dead code for this base image. Will remove or gate it properly in the follow-up when we migrate to the mad-rccl overlay Dockerfile.

  4. No swap verification — Partially addressed. The sha256-based tag means a cache hit now implies the correct library. Full build-time verification (asserting the SHA of every librccl on disk) is exactly what the mad-rccl Dockerfile provides — agreed that's the right end state.

  5. Hardcoded python3.12 / stale staging — Acknowledged. CI is safe (WORK_DIR has github.run_id). For manual runs, --skip-overlay-build bypasses this entirely.

Agreed on the recommendation to migrate to primus_megatron_train_rccl_overlay.ubuntu.amd.Dockerfile from mad-rccl. The gap you noted (it builds from source, CI needs to inject prebuilt TheRock artifacts) is the main blocker — will coordinate on adding that input to the Dockerfile as a follow-up.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Suppressed comments (5)

projects/rccl/ci/scripts/test_madengine.py:786

  • The final header-only run is dropped at EOF. For the BF16/FP8 sequence, an FP8 failure before its first metric leaves only completed BF16 in live_log_runs, allowing the nonzero MADEngine exit code to be overridden to success. Retain the run with completed = false.
        if current and current.get("iter"):
            current["completed"] = current["iter"] == current["total"]
            runs.append(current)

projects/rccl/ci/scripts/test_madengine.py:553

  • The pinned MADEngine treats local_image: true as an image that should already exist on compute nodes; registry images use the registry_image branch. Since this workflow always supplies a GHCR-qualified image, this setting instead attempts a local build and shared-tar synchronization (including serializing the large Primus image to NFS) before falling back to a pull. Generate registry manifests with registry_image and local_image: false, while retaining local mode only when no registry is used.
            image_key: {
                "docker_image": overlay_image,
                "local_image": True,
                "base_docker": overlay_image,
                "build_status": "SKIPPED",

projects/rccl/ci/scripts/test_madengine.py:613

  • MADEngine reads volume mappings from context.docker_mounts (a container-path to host-path mapping); it does not consume this top-level container_mounts list. Consequently the configured /shared:/shared mount is absent from the workload container.
    if workload_config.get("container_mounts"):
        manifest["container_mounts"] = workload_config["container_mounts"]

projects/rccl/ci/scripts/test_pytorch_c10d.py:2

  • This is a duplicate rather than a move: therock-rccl-test-pytorch-distributed.yml:124 still executes .github/scripts/test_pytorch_c10d.py, and the JAX workflow likewise uses .github/scripts/test_jax_collective.py. The old files are not removed, so these new copies are unused and will diverge from fixes applied to the active scripts. Update both workflows and remove the old copies, or omit these additions.
#!/usr/bin/env python3
"""Run PyTorch c10d NCCL distributed tests against CI-built RCCL.

projects/rccl/ci/scripts/test_madengine.py:657

  • The runtime slurm context omits qos, while the pinned MADEngine only merges manifest.deployment_config and ignores the generated manifest's top-level slurm object. Thus Ruby's configured vip_prio QOS never reaches the rendered #SBATCH directives. Thread slurm_qos into this runtime context (or emit and rely on a proper deployment_config).
        "slurm": {
            "partition": slurm_partition,
            "network_interface": network_interface,
            "nodes": nodes,
            "gpus_per_node": gpus_per_node,

Comment on lines +360 to +363
["git", "rev-parse", "HEAD"],
capture_output=True, text=True, check=True,
)
return result.stdout.strip()[:12]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. get_rccl_commit() no longer falls back to git rev-parse HEAD. Resolution order: RCCL_COMMIT_HASH env → GITHUB_RUN_ID → sha256 of librccl.so.

log.info("Overlay image built: %s", tag)

if registry:
push_tag = f"{registry}/rccl-ci:{base_image.replace('/', '-')}-{rccl_commit}"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Push tag now replaces both / and : in base_image, so tags like rocm/primus:v26.4 produce valid Docker references.

Comment on lines +762 to +764
if current and current.get("iter"):
current["completed"] = current["iter"] == current["total"]
runs.append(current)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1b9e135. Header-only runs are now preserved with completed=False. A run with zero iterations no longer passes the all(completed) check.

log.info(regression_msg)

# Step 8: Append result to datastore (one record per precision run)
status = "pass" if exit_code == 0 else "fail"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1b9e135. When precision_results is empty (no parseable metric from either structured or live-log source), the datastore now explicitly records status="fail".

Comment thread .github/workflows/therock-rccl-ci-linux.yml
Comment on lines +216 to +220
therock-test-madengine:
name: "Test MADEngine workloads (scheduled)"
if: ${{ !cancelled() && inputs.amdgpu_families == 'gfx950-dcgpu' && (inputs.event_name == 'schedule' || inputs.event_name == 'workflow_dispatch') }}
needs: [therock-build-linux]
uses: ./.github/workflows/therock-rccl-test-madengine.yml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a3089bc. Added packages: write at both the job level and the top-level caller workflow.

Fix critical issues from mkuznet1 and i-kosarev review:

Manifest structure (generate_manifest):
- Move slurm config into deployment_config.slurm (was root-level,
  ignored by madengine)
- Move env vars into context.docker_env_vars and
  deployment_config.env_vars (root-level env was dead)
- Move container mounts into context.docker_mounts (root-level
  container_mounts was dead)
- Add multiple_results to built_models card so madengine can produce
  per-precision structured output (root cause of empty metrics)
- Set n_gpus to -1 (madengine resolves per-node)
- Leave training_precision empty (card runs both BF16 and FP8)
- Add GLOO_SOCKET_IFNAME to docker_env_vars
- Remove MAD_MULTI_NODE_RUNNER (noise for primus launcher)
- Add qos to deployment_config.slurm (was silently dropped)
- Add docker_run_options from reference template

Simplify run_madengine:
- Remove redundant additional_context (slurm, distributed, env_vars)
  since manifest now has deployment_config in the right place and
  madengine merges it automatically

RCCL commit provenance (get_rccl_commit):
- Remove git rev-parse HEAD fallback — in CI this returns TheRock's
  pinned commit (constant), causing stale image cache hits
- Use RCCL_COMMIT_HASH env, then GITHUB_RUN_ID, then sha256 of
  librccl.so for unique image tags

Push tag format (build_rccl_overlay_image):
- Replace both / and : in base image name to avoid invalid Docker
  reference with two colons

Results parsing:
- Replace parse_perf_csv with parse_perf_results reading
  perf_entry_super.json (31 fixed columns, per-precision rows)
- Fall back to perf.csv with all rows (was collapsing to last row)
- Use per-run status in datastore (was stamping one status on all)
- Copy perf_entry_super files to run artifacts
- Require metric_value for exit code override (was only checking
  iteration count)

Workflow YAML:
- Add secrets: inherit to therock-test-madengine job (HF_TOKEN
  was silently empty)
- Add packages: write permission (needed for GHCR push)
- Upload perf_entry_super files as CI artifacts
@prasanna-amd
prasanna-amd requested a review from a team August 11, 2026 17:55
@prasanna-amd

Copy link
Copy Markdown
Contributor Author

@mkuznet1 @i-kosarev Thanks for the detailed review — addressed the critical items in commit a3089bc:

Fixed in this commit:

  1. Manifest structure rewritedeployment_config.slurm, deployment_config.distributed, deployment_config.env_vars now match the reference template from mad-rccl. Root-level slurm, env, container_mounts keys removed. Mounts moved to context.docker_mounts, env vars to context.docker_env_vars.

  2. multiple_results addedperf_primus-megatron-Llama-3.1-70B.csv on the built_models card. Validated on Ruby: madengine now produces both BF16 and FP8 runs, perf_entry_super.json populated with 8 result rows (4 metrics × 2 precisions). This was the root cause of empty perf files.

  3. n_gpus"-1", training_precision"" — matches MAD card behavior (madengine resolves per-node, card emits both BF16+FP8).

  4. MAD_MULTI_NODE_RUNNER — removed from both env locations.

  5. GLOO_SOCKET_IFNAME — moved into context.docker_env_vars and deployment_config.env_vars (was in dead manifest["env"]).

  6. Commit provenanceget_rccl_commit() no longer falls back to git rev-parse HEAD (which returned TheRock's constant commit). Uses RCCL_COMMIT_HASHGITHUB_RUN_ID → sha256 of librccl.so.

  7. Push tag format — replaces both / and : to avoid invalid Docker reference.

  8. Results parsing — reads perf_entry_super.json first (structured 31-column format), falls back to perf.csv. Uses shutil.copy2 instead of subprocess.run for file copy (fixes OSError vs CalledProcessError mismatch).

  9. Exit code override — now requires metric_value is not None (missing metrics keep the failure).

  10. Workflow YAML — added secrets: inherit + packages: write to madengine job. Added perf_entry_super.* files to upload artifact step.

Deferred to follow-up (not blocking merge):

  • patch_madengine() removal — keeping isolated pending upstream fixes (AICOMNET-366). Ready to drop in one commit once upstream lands.
  • Model alias via models.json — currently uses run.sh substring matching. Will migrate once we have a stable models.json schema to depend on.
  • Live-log scraper removal — kept as fallback safety net. perf_entry_super is now the primary path; live-log only fires if structured data is empty.
  • Per-precision status — still stamps job-wide status across all rows. Will use perf_entry_super per-row status in follow-up.
  • Regression check precision key — fallback path compares against null-precision baseline. Will normalize once we have enough historical data with precision keys.

Validated on Ruby single-node (1N/8GPU): BF16 754.2 TFLOP/s/GPU, FP8 1248.7 TFLOP/s/GPU. All perf files populated correctly.

@mkuznet1 mkuznet1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manifest rewrite matches what madengine actually reads (run_orchestrator.py:225-234) — good turnaround.

Acceptance here is multi-node: Llama-3.1-70B on 2 nodes minimum, green, with metrics in perf_entry_super. The description still lists 2N as follow-up; single-node Ruby does not exercise the path this CI exists for.

perf_entry_super is read but unused — metric, regression and datastore still come from the log scraper (inline).

regression_msg = "; ".join(regression_msgs) if regression_msgs else "N/A"
if is_regression:
exit_code = max(exit_code, 1)
elif metric_value is not None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structured path is an elif: whenever the live log parses, regression compares live-log tokens/s and perf_entry_super is never consulted. The scraper is primary here, not the fallback. perf_entry_super.json already carries per-precision rows in multi_results, which also removes the need to scrape the Running: header at :774 for the precision key.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1b9e135. perf_entry_super.json is now the primary data source. The code builds precision_results from structured data first; live-log scraping only fires as a fallback when not precision_results. Regression and datastore both consume the same precision_results list.

Comment on lines +1195 to +1208
if live_log_runs:
for run in live_log_runs:
append_result(
results_dir,
args.workload,
scale,
run.get("tokens_per_second_per_gpu"),
"pass" if run.get("completed", False) else "fail",
rccl_commit,
extra=extra,
precision=run.get("precision"),
tflops=run.get("tflops_avg"),
tokens_per_sec=run.get("tokens_per_second_per_gpu"),
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for the datastore: every record here comes from the log scraper, including the per-run status added in this commit. perf_entry_super.json carries status per row, so this loop can be driven from it, with the live-log branch kept only for when structured output is missing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1b9e135. Datastore append loop now iterates over precision_results (which comes from perf_entry_super.json when available). Per-row status from the structured output is preserved — no more job-wide stamp.

log.info("Metric %s = %.1f (from structured results)",
row.get("metric", "?"), metric_value)
except (ValueError, TypeError):
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metric_value is overwritten on every row, so this keeps the last of 8 rows (4 metrics x 2 precisions) — TFLOP/s or tokens/s, whichever lands last. Nothing filters on metric == metric_key, and metric_key (:68) is now read nowhere. Same last-row collapse as the old parse_perf_csv, moved into the caller. Filter on row["metric"], key the row by multi_results.precision.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1b9e135. Built a single precision_results list that filters on row["metric"] == metric_key instead of overwriting metric_value on every row. Each matching row becomes a separate entry with its own precision, so BF16 and FP8 values are preserved independently.

@i-kosarev

Copy link
Copy Markdown
Contributor

@mkuznet1 @i-kosarev Thanks for the detailed review — addressed the critical items in commit a3089bc:

Fixed in this commit:

  1. Manifest structure rewritedeployment_config.slurm, deployment_config.distributed, deployment_config.env_vars now match the reference template from mad-rccl. Root-level slurm, env, container_mounts keys removed. Mounts moved to context.docker_mounts, env vars to context.docker_env_vars.
  2. multiple_results addedperf_primus-megatron-Llama-3.1-70B.csv on the built_models card. Validated on Ruby: madengine now produces both BF16 and FP8 runs, perf_entry_super.json populated with 8 result rows (4 metrics × 2 precisions). This was the root cause of empty perf files.
  3. n_gpus"-1", training_precision"" — matches MAD card behavior (madengine resolves per-node, card emits both BF16+FP8).
  4. MAD_MULTI_NODE_RUNNER — removed from both env locations.
  5. GLOO_SOCKET_IFNAME — moved into context.docker_env_vars and deployment_config.env_vars (was in dead manifest["env"]).
  6. Commit provenanceget_rccl_commit() no longer falls back to git rev-parse HEAD (which returned TheRock's constant commit). Uses RCCL_COMMIT_HASHGITHUB_RUN_ID → sha256 of librccl.so.
  7. Push tag format — replaces both / and : to avoid invalid Docker reference.
  8. Results parsing — reads perf_entry_super.json first (structured 31-column format), falls back to perf.csv. Uses shutil.copy2 instead of subprocess.run for file copy (fixes OSError vs CalledProcessError mismatch).
  9. Exit code override — now requires metric_value is not None (missing metrics keep the failure).
  10. Workflow YAML — added secrets: inherit + packages: write to madengine job. Added perf_entry_super.* files to upload artifact step.

Deferred to follow-up (not blocking merge):

  • patch_madengine() removal — keeping isolated pending upstream fixes (AICOMNET-366). Ready to drop in one commit once upstream lands.
  • Model alias via models.json — currently uses run.sh substring matching. Will migrate once we have a stable models.json schema to depend on.
  • Live-log scraper removal — kept as fallback safety net. perf_entry_super is now the primary path; live-log only fires if structured data is empty.
  • Per-precision status — still stamps job-wide status across all rows. Will use perf_entry_super per-row status in follow-up.
  • Regression check precision key — fallback path compares against null-precision baseline. Will normalize once we have enough historical data with precision keys.

Validated on Ruby single-node (1N/8GPU): BF16 754.2 TFLOP/s/GPU, FP8 1248.7 TFLOP/s/GPU. All perf files populated correctly.

I see at least one critical thing that's left, it is in build_rccl_overlay_image() — non-kpack fallback can silently keep stock RCCL (lines 484-486 of test_madengine.py)

  RCCL_REAL=$(readlink -f /opt/rocm/lib/librccl.so 2>/dev/null || \
              find /opt/rocm*/lib -name 'librccl.so.*.*' -not -type l | head -1)
  cp /tmp/rccl_ci/librccl.so "$RCCL_REAL"

readlink -f on a non-existent final path component still exits 0 and prints the resolved (non-existent) path — the || fallback to find never fires. If /opt/rocm/lib/librccl.so isn't present (which happens on rocm/primus-style images, where the runtime lives under _rocm_sdk_libraries/lib / _rocm_sdk_devel/lib instead of the classic /opt/rocm/lib layout), cp just creates a new, unlinked librccl.so file. The actual versioned library the loader resolves (librccl.so.1) is never touched, the build reports success, and the job runs against stock RCCL instead of the target commit — with no error anywhere.

This isn't hypothetical: we hit exactly this failure mode building the Primus overlay on the same base-image family, which is why primus_megatron_train_rccl_overlay.ubuntu.amd.Dockerfile (mad-rccl) has an explicit [ -e /opt/rocm/lib/librccl.so.1 ] check plus a full sweep over every non-symlink librccl.so* on disk instead of resolving a single guessed path.

Given that + the fact that nothing after the cp verifies the swap landed (no readelf/SHA check against the built library, no ldd on the actual runtime lib the process loads), this function can produce a green nightly that measured nothing new. Minimum fix: set -o pipefail, explicit [ -e "$RCCL_REAL" ] || exit 1 before the cp, and a build-time gate that confirms the copied .so's hash/symbol matches the CI-built artifact.

…ne metrics

Drive regression checks and datastore writes from structured output
(perf_entry_super.json) instead of live-log scraping.  Addresses three
review items from mkuznet1 (Aug 12):

- Filter structured rows by metric_key and key by precision instead of
  overwriting metric_value on every row (last-row-wins bug)
- Invert priority: structured results are primary for regression and
  datastore, live-log scraping is fallback only when structured data
  is missing
- Build a single precision_results list consumed by all downstream
  stages (exit-code override, regression, datastore, report)

Also fixes:
- Preserve header-only runs (precision detected, zero iterations) as
  incomplete instead of silently discarding them
- Record status=fail with metric_value=None when no results exist
  (was recording pass with no metric)
@i-kosarev

Copy link
Copy Markdown
Contributor

Work transferred to #10396

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants